test(e2e): add amount args for ERC20 withdraw tests
#3113
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces enhancements to the end-to-end testing framework for ERC20 token workflows. A new test case, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
lumtis
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
e2e/e2etests/test_v2_erc20_withdraw_and_call.go (2)
15-17: Enhance args validation with descriptive error message.While the length validation is correct, consider improving the error message to be more descriptive about the expected format.
-func TestV2ERC20WithdrawAndCall(r *runner.E2ERunner, args []string) { - require.Len(r, args, 1) +// TestV2ERC20WithdrawAndCall tests ERC20 withdrawal with a custom payload +// args[0]: amount - the withdrawal amount in base units (e.g., "1000") +func TestV2ERC20WithdrawAndCall(r *runner.E2ERunner, args []string) { + require.Len(r, args, 1, "TestV2ERC20WithdrawAndCall requires exactly one argument: amount")
19-23: Extract gas limit constant and consider helper function.The gas limit management is correct but could be improved for maintainability and reusability.
+const ( + // DefaultWithdrawGasLimit is the gas limit used for withdrawal operations + DefaultWithdrawGasLimit = 10000000 +) + +// withCustomGasLimit temporarily sets a custom gas limit and restores the original value +func withCustomGasLimit(r *runner.E2ERunner, gasLimit uint64) func() { + previousGasLimit := r.ZEVMAuth.GasLimit + r.ZEVMAuth.GasLimit = gasLimit + return func() { r.ZEVMAuth.GasLimit = previousGasLimit } +} func TestV2ERC20WithdrawAndCall(r *runner.E2ERunner, args []string) { // ... - previousGasLimit := r.ZEVMAuth.GasLimit - r.ZEVMAuth.GasLimit = 10000000 - defer func() { - r.ZEVMAuth.GasLimit = previousGasLimit - }() + defer withCustomGasLimit(r, DefaultWithdrawGasLimit)()cmd/zetae2e/local/v2.go (2)
Line range hint
89-91: Consider tracking the TODO as a GitHub issue.The TODO comment indicates future work to break down this routine. While the current implementation works, tracking this technical debt would help with future maintenance.
Would you like me to create a GitHub issue to track this refactoring task?
Line range hint
15-84: Well-structured test organization with room for enhancement.The current implementation demonstrates good practices:
- Parallel test execution using errgroup
- Clear separation of concerns between gas token and ERC20 workflows
- Visual differentiation using color coding
Consider enhancing the structure by:
- Adding package-level documentation about the test organization strategy
- Considering table-driven tests for similar test cases
- Adding metrics collection for test execution times
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
cmd/zetae2e/local/v2.go(1 hunks)e2e/e2etests/e2etests.go(1 hunks)e2e/e2etests/test_v2_erc20_withdraw_and_call.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
cmd/zetae2e/local/v2.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/e2etests.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_v2_erc20_withdraw_and_call.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (2)
cmd/zetae2e/local/v2.go (1)
42-42: LGTM: Test case addition is well-placed.
The new test case is appropriately placed within the ERC20 happy path test group, maintaining a logical organization of test cases.
e2e/e2etests/e2etests.go (1)
958-960: LGTM: Argument definition follows consistent pattern.
The addition of the amount argument with a default value of "1000" aligns with other ERC20 withdraw tests and improves consistency.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3113 +/- ##
========================================
Coverage 63.29% 63.29%
========================================
Files 422 422
Lines 29959 29959
========================================
Hits 18962 18962
Misses 10157 10157
Partials 840 840 |
Description
Add args
TestV2ERC20WithdrawAndCallNoMessageNameactually expected an arg and it was missing (the test was also not run)TestV2ERC20WithdrawAndCall, despite the previous note, better to add an arg for consistency with other test, it doesn't add overhead as you have a default value and in the future we could whitelist a ERC20 with a high base price (unlikely)Summary by CodeRabbit
New Features
Bug Fixes